Funding signed event by jbesraa · Pull Request #3024 · lightningdevkit/rust-lightning

jbesraa · 2024-04-25T17:25:29Z

codecov-commenter · 2024-04-25T17:36:39Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 80.95238% with 32 lines in your changes missing coverage. Please review.

Project coverage is 89.82%. Comparing base (bfc20f8) to head (8403755).

Files	Patch %	Lines
lightning/src/events/mod.rs	0.00%	20 Missing ⚠️
lightning/src/ln/channelmanager.rs	87.17%	6 Missing and 4 partials ⚠️
lightning/src/ln/functional_tests.rs	95.65%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #3024    +/-   ##
========================================
  Coverage   89.81%   89.82%            
========================================
  Files         121      121            
  Lines       99515    99657   +142     
  Branches    99515    99657   +142     
========================================
+ Hits        89381    89518   +137     
- Misses       7519     7530    +11     
+ Partials     2615     2609     -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

benthecarman · 2024-04-26T05:26:23Z

+		/// [`ChannelManager::create_channel`]: crate::ln::channelmanager::ChannelManager::create_channel
+		/// [`ChannelManager::accept_inbound_channel`]: crate::ln::channelmanager::ChannelManager::accept_inbound_channel
+		/// [`UserConfig::manually_accept_inbound_channels`]: crate::util::config::UserConfig::manually_accept_inbound_channels
+		user_channel_id: u128,


You should give the funding tx to broadcast

TheBlueMatt · 2024-05-06T22:30:02Z

 	/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
 	pub accept_mpp_keysend: bool,
+	/// broadcast funding transaction manually
+	pub manually_broadcast_outbound_channels: bool,


Hmm, is it best to do this via a config or via a different method to pass LDK the funding transaction? A config seems to imply people might want to just turn this on, but really its a totally differnet flow, and maybe thus makes sense as a different method since it'll change the programming entirely?

Ok ill give that a try to see how that works.

What do you think about the next event after FundingSigned? In the current implementation its going from FundingSigned to ChannelReady ie skips the ChannelPending. I did that because in the code ChannelPending = "funding is ready and signed, just wait for confirmations", but that is not accurate for all cases. in payjoin scenario we never really know that the funding is fully signed and broadcasted.

Hmm, we should probably keep ChannelPending, I think - its pending from the lightning POV, just something else is dealing with broadcasting.

jbesraa · 2024-05-31T10:58:57Z

Done few things:

CherryP Add unsafe_funding_transaction_generated #3056 here
Removed FUNDING_SIGNED channel state
Removed config manually_broadcast_.. argument

Yet to do:

Complete the docs
Fix CI
Improve/fix naming around the new property in ChannelContext

jbesraa · 2024-06-03T10:37:05Z

@TheBlueMatt @benthecarman this is ready for review

tnull

Did an intial pass.

tnull · 2024-06-03T12:59:44Z

+	///
+	/// Upon receiving this event, it is your responsibility to broadcast the funding transaction.
+	///
+	/// [`ChannelManager::unsafe_manual_funding_transaction_generated`]:


nit: These references need to live on a single line each, I believe.

tnull · 2024-06-03T13:00:20Z

+	/// crate::ln::channelmanager::ChannelManager::unsafe_manual_funding_transaction_generated
+	/// [`ChannelManager::funding_transaction_generated`]:
+	/// crate::ln::channelmanager::ChannelManager::funding_transaction_generated
+	FundingSigned {


Can we also include the counterparty_node_id and former_temporary_channel_id here?

tnull · 2024-06-03T13:03:03Z

 	/// network. For 0conf channels it will be immediately followed by the corresponding
 	/// [`Event::ChannelReady`] event.
+	///
+	/// [`ChannelManager::unsafe_manual_funding_transaction_generated`]:


nit: Same as above: move to a single line.

tnull · 2024-06-03T13:05:48Z

 				f()
 			},
+			41u8 => {
+					let mut channel_id = ChannelId::new_zero();


Let's use RequiredWrapper(None) instead of initializing these defaults.

tnull · 2024-06-03T13:06:49Z

+	/// The funding transaction can be accessed through the [`Event::FundingSigned`] event.
+	///
+	/// [`Event::FundingSigned`]: crate::events::Event::FundingSigned
+	manually_broadcast_outbound_channels: Option<()>,


Why is this an Option<()> rather than a bool? Also, given this is a flag indicating whether we broadcast the transaction, should this start with did_ or is_?

The Option<()> is mainly because other variables like is_batch_funding and local_initiated_shutdown are defined with Option so I was trying to follow that. I was sure it saves a byte or two but it seems it doesnt. happy to change it back to bool

tnull · 2024-06-03T13:19:22Z

 	pub fn is_funding_broadcast(&self) -> bool {
-		!self.channel_state.is_pre_funded_state() &&
-			!matches!(self.channel_state, ChannelState::AwaitingChannelReady(flags) if flags.is_set(AwaitingChannelReadyFlags::WAITING_FOR_BATCH))
+			!self.channel_state.is_pre_funded_state() 


nit: Remove the superfluous indent. Generally, since nothing seems to have actually changed here, might be preferable to not touch this code at all?

tnull · 2024-06-03T13:22:05Z

-				(matches!(self.context.channel_state, ChannelState::AwaitingChannelReady(flags) if !flags.is_set(AwaitingChannelReadyFlags::WAITING_FOR_BATCH)) ||
-				matches!(self.context.channel_state, ChannelState::ChannelReady(_)))
-			{
+			if self.context.manually_broadcast_outbound_channels.is_none() 


Same here: if nothing actually changed, it would be preferable to not touch it, or do the reformatting in a separate commit.

tnull · 2024-06-03T13:33:44Z

 		}
+		if result != Ok(()) { return result; }
+
+		return self.batch_funding_transaction_generated_intern(temporary_channels, funding_transaction, false);


Could just avoid the above if and do

Suggested change

return self.batch_funding_transaction_generated_intern(temporary_channels, funding_transaction, false);

result.and(self.batch_funding_transaction_generated_intern(temporary_channels, funding_transaction, false))

here.

tnull · 2024-06-03T13:39:49Z

 	// Checks whether we should emit a `ChannelPending` event.
 	pub(crate) fn should_emit_channel_pending_event(&mut self) -> bool {
-		self.is_funding_broadcast() && !self.channel_pending_event_emitted
+		self.is_funding_broadcast() && !self.channel_pending_event_emitted && self.manually_broadcast_outbound_channels.is_none()


Hmm, any way we can keep ChannelPending?

I think we have two options here:

Let the user tell us they broadcasted the transaction and as a result we emit ChannelPending

Track the chain and check when the transaction is in the mempool and emit ChannelPending

First option is not always doable from the user perspective, for example in payjoin scenario you dont really know when the tx will be broadcasted. wdyt?

We could also just generate it when we generate the FundingSigned event, right?

That would be the easiest if we are comfortable emitting both events simultaneously

I assume we would, yes.

TheBlueMatt · 2024-06-03T15:32:39Z

 		/// [`UserConfig::manually_accept_inbound_channels`]: crate::util::config::UserConfig::manually_accept_inbound_channels
 		user_channel_id: u128,
 	},
+	/// Used to indicate that the counterparty node has sent us `funding_signed` message but we are


This is great for people who work on the lightning protocol, but we try to keep our docs understandable for those who do not as well. Instead, we should phrase simply as "the counterparty has provided the signature(s) required to recover our funds if they go offline and the funding transaction is now broadcastable" or so.

TheBlueMatt · 2024-06-03T15:33:22Z

+	/// crate::ln::channelmanager::ChannelManager::unsafe_manual_funding_transaction_generated
+	/// [`ChannelManager::funding_transaction_generated`]:
+	/// crate::ln::channelmanager::ChannelManager::funding_transaction_generated
+	FundingSigned {


FundingSigned is a reference to a lightning-internal name, but isn't reflective of what has happened (the funding transaction itself was not signed, rather the first commitment transaction was) nor what has changed from the user's perspective (the funding transaction is now broadcastable).

TheBlueMatt · 2024-06-03T15:46:10Z

 }
+macro_rules! emit_funding_signed_event {
+	($locked_events: expr, $channel: expr) => {
+		let should_emit = $channel.context.should_emit_funding_signed_event();


Sadly, like with actual broadcasting, we need to not emit the event until the channelmonitor update that has the new signature gets written to disk (as otherwise its not, actually, safe to broadcast yet). In general, there's a lot of machinery here to duplicate what we do with the funding transaction broadcast...ISTM we could better simplify this code by just checking if the channel is "manual broadcast" whenever we'd otherwise broadcast the funding tx and convert to an event (if relevant).

Following your suggestion above with emitting both events simultaneously, could we have the same should_emit_..(roughly, excluding the respective variable is_emitted..) function for FundingSigned and ChannelPending events?

Possibly, but it certainly seems to match up tighter with when LDK decides to broadcast, rather than when we decide to release the pending event.

tnull

Basically LGTM, just a few nits.

tnull · 2024-06-19T11:30:06Z

+		channel_id: ChannelId,
+		/// The `user_channel_id` value passed in to [`ChannelManager::create_channel`] for outbound
+		/// channels, or to [`ChannelManager::accept_inbound_channel`] for inbound channels if
+		/// [`UserConfig::manually_accept_inbound_channels`] config flag is set to true. Otherwise


nit:

Suggested change

/// [`UserConfig::manually_accept_inbound_channels`] config flag is set to true. Otherwise

/// [`UserConfig::manually_accept_inbound_channels`] config flag is set to `true`. Otherwise

tnull · 2024-06-19T11:34:11Z

 	counterparty_forwarding_info: Option<CounterpartyForwardingInfo>,

 	pub(crate) channel_transaction_parameters: ChannelTransactionParameters,
+	/// The transaction which funds this channel. Note that for manually-funded channels(i.e,


nit:

Suggested change

/// The transaction which funds this channel. Note that for manually-funded channels(i.e,

/// The transaction which funds this channel. Note that for manually-funded channels (i.e.,

tnull · 2024-06-19T11:35:18Z

+	/// This flag indicates that it is the user's responsibility to validated and broadcast the
+	/// funding transaction.
+	///
+	/// [`Event::FundingTxBroadcastSafe`]: crate::events::Event::FundingTxBroadcastSafe


Drop unused doc link.

Suggested change

/// [`Event::FundingTxBroadcastSafe`]: crate::events::Event::FundingTxBroadcastSafe

tnull · 2024-06-19T11:37:06Z

 			(45, cur_holder_commitment_point, option),
 			(47, next_holder_commitment_point, option),
 			(49, self.context.local_initiated_shutdown, option), // Added in 0.0.122
+			(51, is_manual_broadcast, option),


Should we also add // Added in .. comments for these fields, like the ones above?

tnull · 2024-06-19T11:39:01Z

+				user_channel_id: $channel.context.get_user_id(),
+				funding_txo: $funding_txo,
+				counterparty_node_id: $channel.context.get_counterparty_node_id(),
+				former_temporary_channel_id: $channel.context.temporary_channel_id().expect("Unreachable: FundingTxBroadcastSafe event feature added to channel establishment process in LDK v0.124.0 where this should never be None."),


nit: Add newline before .expect

tnull · 2024-06-19T11:39:34Z

 	}

+
+	/// Unsafe: This method does not check the validatiy of the provided output. It is the


Suggested change

/// Unsafe: This method does not check the validatiy of the provided output. It is the

/// **Unsafe**: This method does not check the validity of the provided output. It is the

TheBlueMatt · 2024-06-19T18:14:43Z

+						emit_funding_tx_broadcast_safe_event!(pending_events, channel, funding_txo.into_bitcoin_outpoint())
+					},
+					None => {
+						log_error!(logger, "Channel resumed without a funding txo, this should never happen!");


I think we can safely debug_assert!(false); here :)

TheBlueMatt · 2024-06-19T18:15:19Z

 	internal_create_funding_transaction(node, expected_counterparty_node_id, expected_chan_value, expected_user_chan_id, true)
 }

+pub fn create_funding_tx_without_witness_data<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>,


Given the method no longer even takes a full tx this seems like overkill :). It can now be deduced entirely from the API itself that the ChannelManager cannot possibly test for witnesses.

TheBlueMatt · 2024-06-19T18:16:25Z

+	/// broadcasted and you are expected to broadcast it manually when receiving the
+	/// [`Event::FundingTxBroadcastSafe`] event.
+	///
+	/// Returns an [`APIError::APIMisuseError`] if no output was found which matches the parameters


This can't happen anymore ;)

This line still needs removing.

TheBlueMatt · 2024-06-19T18:16:53Z

+	/// Returns [`APIError::ChannelUnavailable`] if a funding transaction has already been provided
+	/// for the channel or if the channel has been closed as indicated by [`Event::ChannelClosed`].
+	///
+	/// May panic if the output found in the funding transaction is duplicative with some other


Suggested change

/// May panic if the output found in the funding transaction is duplicative with some other

/// May panic if the funding output is duplicative with some other

TheBlueMatt · 2024-06-19T18:17:21Z

 	}

+
+	/// **Unsafe**: This method does not check the validity of the provided output. It is the


Ideally we should say what that validity requirement is. Specifically its very easy for a user to not check that inputs are segwit, so we should mention that explicitly imo.

jbesraa · 2024-06-20T09:26:15Z

@TheBlueMatt please see last commit for changes.

~~I am not sure now we still need funding_transaction_generated_intern and funding_transaction_generated ? wdyt?~~

TheBlueMatt

I am not sure now we still need funding_transaction_generated_intern and funding_transaction_generated ? wdyt?

Not quite sure what your question is?

TheBlueMatt · 2024-06-20T14:01:54Z


+
+	/// **Unsafe**: This method does not check the validity of the provided output. It is the
+	/// caller's responsibility to ensure that. Its important to validate you are using SegWit


This reads as if you're supposed to check that the funding output is segwit, which is true but users shouldn't need to worry too much about it unless they screw up really bad. Instead, they need to check that all inputs are segwit.

TheBlueMatt

LGTM, modulo a few nits.

TheBlueMatt · 2024-07-08T16:32:21Z


+
+	/// **Unsafe**: This method does not validate the spent output. It is the caller's
+	/// responsibility to ensure the spent output are SegWit, as well as making sure the funding


Suggested change

/// responsibility to ensure the spent output are SegWit, as well as making sure the funding

/// responsibility to ensure the spent outputs are SegWit, as well as making sure the funding

TheBlueMatt · 2024-07-08T16:36:15Z

+	/// broadcasted and you are expected to broadcast it manually when receiving the
+	/// [`Event::FundingTxBroadcastSafe`] event.
+	///
+	/// Returns an [`APIError::APIMisuseError`] if no output was found which matches the parameters


This line still needs removing.

TheBlueMatt · 2024-07-08T16:37:44Z

+					err: "Transaction had more than 2^16 outputs, which is not supported".to_owned()
 				}));
 			}
+			{


nit: you can drop the unnecessary scope here if you want now.

jbesraa · 2024-07-08T17:04:40Z

@TheBlueMatt Addressed all comments

The `FundingTxBroadcastSafe` event indicates that we have received `funding_signed` message from our counterparty and that you should broadcast the funding transaction. This event is only emitted if upon generating the funding transaction you call `ChannelManager::unsafe_manual_funding_transaction_generated` that will emit this event instead of `ChannelPending` event. `ChannelManager::unsafe_manual_funding_transaction_generated` wont check if the funding transaction is signed, those its unsafe. It is manual because you are responsibile on broadcasting the transaction once the event is received.

TheBlueMatt

Oops, noticed one more issue, but we can address it in a followup in the same release cause this PR has gotten somewhat long in the tooth.

TheBlueMatt · 2024-07-09T15:22:56Z


 	pub(crate) channel_transaction_parameters: ChannelTransactionParameters,
+	/// The transaction which funds this channel. Note that for manually-funded channels (i.e.,
+	/// is_manual_broadcast is true) this will be a dummy empty transaction.


Oops, one more issue here, the DiscardFunding event will now expose the dummy transaction here. We should hide that by adding an enum and including the funding OutPoint there instead.

tnull · 2024-07-22T13:47:57Z

Excuse the delay here, landing this.

benthecarman reviewed Apr 26, 2024

View reviewed changes

jbesraa force-pushed the funding-signed-event branch 3 times, most recently from e9ffefc to d48453a Compare April 29, 2024 09:21

jbesraa marked this pull request as ready for review April 29, 2024 11:51

jbesraa commented Apr 29, 2024

View reviewed changes

Comment thread lightning/src/ln/channelmanager.rs Outdated

jbesraa commented Apr 29, 2024

View reviewed changes

Comment thread lightning/src/ln/channel.rs Outdated

jbesraa force-pushed the funding-signed-event branch 2 times, most recently from cb5e32f to 20313c8 Compare April 29, 2024 11:59

jbesraa commented Apr 29, 2024

View reviewed changes

Comment thread lightning/src/ln/functional_tests.rs Outdated

TheBlueMatt reviewed May 6, 2024

View reviewed changes

TheBlueMatt mentioned this pull request May 13, 2024

Add unsafe_funding_transaction_generated #3056

Closed

TheBlueMatt reviewed May 28, 2024

View reviewed changes

Comment thread lightning/src/ln/channel.rs Outdated

jbesraa force-pushed the funding-signed-event branch 2 times, most recently from 5f7cf6c to 1aa96df Compare May 31, 2024 10:42

jbesraa force-pushed the funding-signed-event branch 5 times, most recently from bed2ba6 to b3d033d Compare June 3, 2024 10:31

jbesraa mentioned this pull request Jun 3, 2024

Allow node wallet to receive payjoin transactions lightningdevkit/ldk-node#301

Closed

tnull reviewed Jun 3, 2024

View reviewed changes

TheBlueMatt reviewed Jun 3, 2024

View reviewed changes

jbesraa force-pushed the funding-signed-event branch 4 times, most recently from 0540f52 to b21c02c Compare June 4, 2024 11:18

TheBlueMatt assigned tnull Jun 18, 2024

tnull reviewed Jun 19, 2024

View reviewed changes

jbesraa force-pushed the funding-signed-event branch from dad9947 to d0e779f Compare June 19, 2024 11:59

TheBlueMatt reviewed Jun 19, 2024

View reviewed changes

jbesraa force-pushed the funding-signed-event branch 2 times, most recently from e61909f to d359dab Compare June 20, 2024 09:24

TheBlueMatt reviewed Jun 20, 2024

View reviewed changes

jbesraa force-pushed the funding-signed-event branch 2 times, most recently from a563480 to 06ba332 Compare July 1, 2024 06:43

TheBlueMatt previously approved these changes Jul 8, 2024

View reviewed changes

jbesraa dismissed TheBlueMatt’s stale review via 150e748 July 8, 2024 17:03

jbesraa force-pushed the funding-signed-event branch from 06ba332 to 150e748 Compare July 8, 2024 17:03

jbesraa force-pushed the funding-signed-event branch from 150e748 to 8403755 Compare July 8, 2024 17:10

TheBlueMatt approved these changes Jul 9, 2024

View reviewed changes

TheBlueMatt mentioned this pull request Jul 9, 2024

#3024 exposes dummy tx in DiscardFunding #3164

Closed

tnull approved these changes Jul 22, 2024

View reviewed changes

tnull merged commit 3733103 into lightningdevkit:main Jul 22, 2024

tnull mentioned this pull request Feb 27, 2025

Add unsafe_funding_transaction_generated that wont check for fully signed inputs #3022

Closed

	return self.batch_funding_transaction_generated_intern(temporary_channels, funding_transaction, false);
	result.and(self.batch_funding_transaction_generated_intern(temporary_channels, funding_transaction, false))

	/// [`UserConfig::manually_accept_inbound_channels`] config flag is set to true. Otherwise
	/// [`UserConfig::manually_accept_inbound_channels`] config flag is set to `true`. Otherwise

	/// The transaction which funds this channel. Note that for manually-funded channels(i.e,
	/// The transaction which funds this channel. Note that for manually-funded channels (i.e.,

		}


		/// Unsafe: This method does not check the validatiy of the provided output. It is the

	/// Unsafe: This method does not check the validatiy of the provided output. It is the
	/// Unsafe: This method does not check the validity of the provided output. It is the

	/// May panic if the output found in the funding transaction is duplicative with some other
	/// May panic if the funding output is duplicative with some other



		/// Unsafe: This method does not check the validity of the provided output. It is the
		/// caller's responsibility to ensure that. Its important to validate you are using SegWit



		/// Unsafe: This method does not validate the spent output. It is the caller's
		/// responsibility to ensure the spent output are SegWit, as well as making sure the funding

Uh oh!

Conversation

jbesraa commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jbesraa commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbesraa commented Jun 3, 2024

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbesraa Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbesraa commented Apr 25, 2024 •

edited

Loading

codecov-commenter commented Apr 25, 2024 •

edited

Loading

jbesraa commented May 31, 2024 •

edited

Loading

jbesraa Jun 3, 2024 •

edited

Loading

jbesraa commented Jun 20, 2024 •

edited

Loading